Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a number of small issues in rmw_zenoh_cpp #106

Merged
merged 9 commits into from
Feb 8, 2024
Merged

Conversation

clalancette
Copy link
Collaborator

This PR is a smorgasbord of small fixes around the tree. It is probably easiest to look at the individual patches, but in short:

  • Add a small optimization where we keep a running total of the number of nodes in the graph. This speeds up get_nodes_names for large graphs, since we don't have to iterate over the namespaces anymore.
  • While doing the above, I noticed that we never removed namespaces once they were added. I added a patch so that if there are no nodes left in the namespace after this one is removed, we also remove the namespace.
  • I removed a few old TODO comments.
  • I removed the PublishToken implementation. While it is somewhat nice to have, it is a bit messy and I think we are going to have to do something more substantial when we try to support zenoh-pico. And this code is still in the git history.
  • Remove some hand-coded rcutils_strdup implementations. The original reason for that hand-coding is gone, so we can just use rcutils_strdup now.
  • Speed up ros_topic_name_to_zenoh_key. In particular, std::to_string is much faster than std::stringstream, so use that.
  • Use RAII for saved_msg_data. That way we don't have to remember to z_drop the sample.
  • Implement rmw_compare_gids_equal, which just gets us one step closer to having a complete implementation.

That way we don't have to compute it when the RMW layer
asks for it.

Signed-off-by: Chris Lalancette <[email protected]>
If all nodes from a particular namespace have been removed,
make sure to remove that namespace as well.

Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
While it is a nice idea, there is more than just liveliness
that will prevent us from using zenoh-pico currently.  Remove
this code until we decide to officially support zenoh-pico.

Signed-off-by: Chris Lalancette <[email protected]>
We know that the pointer won't be NULL by that point,
so we don't need the check.

Signed-off-by: Chris Lalancette <[email protected]>
There is really no need for it; we can just use rcutils_strdup
instead.

Signed-off-by: Chris Lalancette <[email protected]>
Use stringstream here is really slow; we can speed it
up by about 100% by switching to std::to_string instead.

Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
@clalancette clalancette requested a review from Yadunund February 8, 2024 03:10
Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cleanup!

@Yadunund Yadunund merged commit 60c4380 into rolling Feb 8, 2024
5 checks passed
@delete-merged-branch delete-merged-branch bot deleted the clalancette/fixes branch February 8, 2024 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants